Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Privacy Panel new pull request #25688

Conversation

martapiekarska
Copy link
Contributor

This is a new pull request. We had to remove the old one due to big problems with git and some Schroedinger patches....

@try-server-hook
Copy link

martasect martasect started tests. Results

lon = pos.coords.longitude;

this._sendSMS(number, navigator.mozL10n.get('sms-locate', {
coords: lat + ',' + lon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even better to pass lat and lon separately and let localizers use the local presentation form for coordinates.

@etiennesegonzac
Copy link
Contributor

Looks like we're missing all the binay files again... (not completely blocking)

}.bind(this), 3000);
}.bind(this);

Commands.invokeCommand('ring', [86400, ringReply]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're then multiplying by 1000 the duration in commands.js...
so my phone will be ringing for more than 2 years I think :) not so sure about the UX...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They wanted the phone to ring infinetly...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we stop when the phone is unlocked?

note: anybody can long press the power button, restart the phone and stop the ringer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long the limit should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected to no multiplication

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys, this value represents only 1 day :) not 2 years.
60 sec * 60 min * 24h = 86400
and javascripts timestamps are with miliseconds so thats why this was multipled by 1000

Ad2 New patch should also have ability to stop ringer.

@try-server-hook
Copy link

martasect martasect started tests. Results

@try-server-hook
Copy link

martasect martasect started tests. Results

@martapiekarska
Copy link
Contributor Author

I have added the binaries/

@@ -0,0 +1,28 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we can rm -r apps/privacy-panel/build

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, project will work, but build config allow us to combine all files into one file thanks to requirejs optimizer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the gaia build system also does this kind of optimizations, so it might be redundant.
anyway, I'm fine with it if we have make test-perf results showing that it helps, otherwise we shouldn't add the complexity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@etiennesegonzac
Copy link
Contributor

we will need marionette tests for the interaction with the settings app.

  • launching from the settings app then going back
  • being sent to the passcode screen in the system app when enabling RPP

}

.icon-gt {
background-image: url("images/gt.png");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably want to test background-size for those to prevent the weird "big icon rendering" on the main screen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you elaborate on that, please? How would you like to test this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the intended design right?
2014-11-05-16-17-22

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, test UI's like this using pseudolocales. They add ~30% to the string length which is what usually makes the text fit for most locales. The middle string seems like it may not fit.

@try-server-hook
Copy link

martasect martasect started tests. Results

realMozSettings = navigator.mozSettings;
navigator.mozSettings = mozSettings;

navigator.mozL10n = { get: function() {} };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that enables you to remove mozL10n from this tests and just test for L10nAttrs object or string being passed into _sendSMS.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@try-server-hook
Copy link

martasect martasect started tests. Results

// lets wait till Promise resolve privacy-panel app.
this.subject._getApp().then(function() {
done();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we have to add one more function to avoid this _getApp() is rejected (This would not happen and should not happen), and if that happen, we should throw out error. And no matter _getApp() is resolved or rejected, we should always call done to make sure tests can be run.

Please check https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/test/unit/modules/apps_cache_test.js#L48

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@try-server-hook
Copy link

martasect martasect started tests. Results

@try-server-hook
Copy link

martasect martasect started tests. Results

@try-server-hook
Copy link

martasect martasect started tests. Results

@try-server-hook
Copy link

martasect martasect started tests. Results

@try-server-hook
Copy link

martasect martasect started tests. Results

@KevinGrandon
Copy link
Contributor

Closing in favor of: 0f7bb15

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants